fix: run error handling, rollback visibility, stale status indicator, and spurious notifications#69
Conversation
… and spurious notifications
Bug 1: Generic "Check server logs" error on model run failure
- execute_run_command caught all exceptions with a blanket `except Exception`
and returned "Model execution failed. Check server logs for details."
- Now catches VisitranBaseExceptions/VisitranBackendBaseException separately
and returns the detailed error via err.error_response() (e.g. model name,
actual error cause, remediation hint)
- Generic Exception fallback now returns 500 with a clearer message
Bug 2: Rollback button never appearing on run failure
- execute_visitran_run_command sets is_rollback on the exception's error_args,
but error_response() only exposed it nested inside message_args
- Frontend checks error.response.data.is_rollback (top level)
- Fixed both VisitranBaseExceptions and VisitranBackendBaseException
error_response() to surface is_rollback at the top level when present
Bug 3: Stale failure indicator in explorer after fixing model
- run_status and failure_reason on ConfigModels were only updated during
execute_graph and never cleared when the user modified the model spec
- After deleting a bad transformation, the explorer still showed the old
red dot with the previous error
- Added _reset_model_run_status in NoCodeModel._validate_and_update_model
to clear FAILED status back to NOT_STARTED when the spec changes
Bug 4: "Something went wrong" popup on frontend console errors
- .catch(error => notify({ error })) catches both API errors and JS
runtime errors (e.g. TypeError from unexpected response shape)
- JS runtime errors have no error.response, so notification-service
fell through to the generic "Something went wrong" description
- Now skips the popup for non-API errors and logs them to console instead
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
| Filename | Overview |
|---|---|
| backend/backend/application/context/no_code_model.py | Adds _reset_model_run_status guarded by NON_EXECUTION_CONFIG_TYPES and broad except Exception so status reset never poisons the outer update result. Previous thread concerns fully addressed. |
| backend/backend/core/routers/execute/views.py | Exception handling split into domain exceptions (preserving HTTP status via to_response()) and fallback except Exception returning 500. Previous status-code concern addressed. |
| backend/backend/errors/visitran_backend_base_exceptions.py | Surfaces is_rollback at top level of error_response() when present in _msg_args; backward-compatible since message_args is still included. |
| backend/visitran/errors/base_exceptions.py | Same is_rollback promotion as the backend base exception; mirrors the sibling class change correctly. |
Sequence Diagram
sequenceDiagram
participant FE as Frontend
participant View as execute_run_command
participant App as ApplicationContext
participant NC as NoCodeModel
participant DB as ConfigModels (DB)
FE->>View: POST /execute/run
View->>App: execute_visitran_run_command()
alt Success
App-->>View: OK
View->>App: backup_current_no_code_model()
View-->>FE: 200 {status: success}
else VisitranBaseExceptions / VisitranBackendBaseException
App-->>View: raises domain exception
View->>View: err.to_response() or err.error_response()
View-->>FE: 4xx/5xx with error_message + is_rollback (top-level)
else Unexpected Exception
App-->>View: raises generic Exception
View-->>FE: 500 {status: failed, error_message: Unexpected error}
end
FE->>View: PUT /model (spec change)
View->>NC: _validate_and_update_model(config_type)
NC->>NC: update_model()
alt config_type not in NON_EXECUTION_CONFIG_TYPES
NC->>DB: get model instance
DB-->>NC: model_instance
alt run_status == FAILED
NC->>DB: save(run_status=NOT_STARTED, failure_reason=None)
end
end
NC-->>View: result
View-->>FE: updated model
Reviews (5): Last reviewed commit: "fix: catch all exceptions in _reset_mode..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add !error.request check so Axios network errors (connection refused, server down) still show notifications — only suppress true JS runtime errors that have neither .response nor .request - Use err.to_response() for VisitranBackendBaseException to preserve the exception's own HTTP status code (403, 404, etc.) instead of hardcoding 400 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Presentation changes (column sort/hide/order) don't affect model execution, so they should not clear a FAILED status indicator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
abhizipstack
requested review from
Deepak-Gnanasekar,
tahierhussain and
wicky-zipstack
There was a problem hiding this comment.
@abhizipstack 4 distinct bugs each with a clear root cause and minimal fix.
views.py — error handling split
Good. The existing exception classes already have formatted error messages — no reason to discard them with a generic catch-all. Minor: the hasattr(err, 'to_response') check on L70 — if no exception class implements to_response(), this is dead code. Consider removing it.
base_exceptions.py / visitran_backend_base_exceptions.py — rollback visibility
Clean fix. Frontend already expects is_rollback at top level — this surfaces it without changing the API contract.
no_code_model.py — stale failure reset
Logic is sound — only resets FAILED → NOT_STARTED, leaves SUCCESS/RUNNING alone. The config_type not in ("presentation",) guard is a nice touch. Minor: consider a named constant like NON_EXECUTION_CONFIG_TYPES = {"presentation"} so it's more discoverable if more types get added later.
notification-service.js — runtime error suppression
The !error.request check correctly distinguishes JS runtime errors from network errors (which have .request set by Axios). Network failures will still show a notification. Looks correct.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@wicky-zipstack Thanks for the review!
FE notification change — Reverted from this PR, will handle separately. |
A DB error during status reset should not fail the outer model update. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
What
Fixes 4 bugs reported from QA staging testing:
Why
is_rollbackbeing nested insidemessage_argsinstead of at the top level of the error responseHow
Bug 1 — Generic error message (
backend/backend/core/routers/execute/views.py):except Exceptioninto two:VisitranBaseExceptions/VisitranBackendBaseException(returnserr.error_response()with detailed markdown error) and genericException(returns 500 with clearer fallback message)err.to_response()when available to preserve the exception's own HTTP status codeBug 2 — Rollback button (
backend/visitran/errors/base_exceptions.py,backend/backend/errors/visitran_backend_base_exceptions.py):is_rollbackat the top level oferror_response()when present in_msg_args, matching what the frontend expects aterror.response.data.is_rollbackBug 3 — Stale failure indicator (
backend/backend/application/context/no_code_model.py):_reset_model_run_status()called from_validate_and_update_model()to clearFAILEDstatus back toNOT_STARTEDwhen the model spec changesconfig_typeso presentation-only changes (sort/hide/order) do NOT reset the statusBug 4 — Spurious popups (
frontend/src/service/notification-service.js):.responseand no.request) with no explicit message — logs to console instead.request)Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No. All changes are backward-compatible:
is_rollbackat top level, keepsmessage_argsintact)handle_http_requestdecorator already setsis_rollbackat the top level for other endpoints — this aligns the run endpoint behaviorFAILEDstate, does not affectSUCCESSorRUNNINGDatabase Migrations
None
Env Config
None
Relevant Docs
Related Issues or PRs
Dependencies Versions
No changes
Notes on Testing
LN(0)) — verify error toast shows specific error with model name, not "Check server logs"Screenshots
Checklist
I have read and understood the Contribution Guidelines.